Improve explain by ordering predicates by bmc99 · Pull Request #11903 · prestodb/presto

bmc99 · 2018-11-12T21:00:14Z

kokosing · 2018-11-13T06:38:10Z

Can you please add a test to com.facebook.presto.tests.TestQueryPlanDeterminism that would fail without your change here?

@kokosing Grzegorz, this functionality requires partitioned tables. TestQueryPlanDeterminism doesn't have any.

Isn't TPCH "partitioned"? See orders.orderstatus, part.container and part.type

@kokosing That's right. I didn't notice. I couldn't reproduce the issue using these tables though. In general, this has been tricky to reproduce. One repro we have involves a complicated query and a table with 7 partition keys.

mbasmanova · 2018-11-13T15:41:58Z

@bmc99 Mithun, the code changes look good. Let's improve the commit message a bit. How about this:

Print partition columns in order in explain plans

To improve plan stability (e.g. make sure explain plans for the same query match) print 
partition columns for a table in alphabetical order.

bmc99 · 2018-11-13T16:33:27Z

I have updated the commit message.

martint · 2018-11-13T16:40:34Z

print partition columns for a table in alphabetical order.

Partition columns is a connector-specific concept (in particular, just Hive supports it). This engine-level change applies to any column that was pushed down into the connector during query planning.

mbasmanova · 2018-11-13T16:55:04Z

@martint Martin, thanks for clarifying. What would be a good way to describe this change then? Will this work?

Print pushed-down predicates in order in explain plans

To improve plan stability (e.g. make sure explain plans for the same query match) print
pushed-down predicates in alphabetical order.

kokosing

% Martin's comments

sopel39 · 2018-11-15T11:50:55Z

This seems like fixing the symptoms. Do you know what is the root cause of non-determinism?

I have considered fixing the predicate order at the place they are added as one of the option. But, I did not see a need for the the predicates to be in order anywhere else except in the explain plan. So, I have decided to fix it a that layer.

Are there any requirements/need to have the predicates ordered throughout? Can you please elaborate as to why you think there is a root issue other than explain plan?

TableScanNode#getCurrentConstraint is used in multiple places in the code. It could be that predicate order matters in those places (e.g: when converting predicate back to filter expression). There are quite sophisticated methods that process TupleDomain.

@sopel39 Karol, it feels like exploring what other places might need fixing is beyond the scope of this change.

Maybe it's just TupleDomain itself. For instance I found TupleDomain#simplify which uses nondeterministic HashMap collector. Another one is: TupleDomain#intersect (uses HashMap). Another one: TupleDomain#columnWiseUnion (uses HashMap). Another one: TupleDomain#transform. Instead of HashMap we could use LinkedHashMap that preserves order.

There are probably few such places to be fixed. Then explicit sorting in explain should not be needed and plan itself would be deterministic (not just explain).

sopel39 · 2018-11-15T11:51:51Z

This change doesn't improve plan stability (in actual plan partition columns are still in unspecified order, right?), but explain stability

I agree that the change stabilizes the explain; Plan stability is not an issue as it relates to unspecified order of the predicates so the verbiage can be changed to use the word explain instead of plan.

To improve explain plan stability (e.g. make sure explain plans for the same query match) print pushed-down predicates in alphabetical order.

martint

I agree with Karol that this is just fixing a symptom. We should fix the underlying cause, instead.

mbasmanova · 2018-11-16T21:44:03Z

@martint @sopel39 Just for my understanding, isn't it fragile to rely on a map to be in a particular order? The interface itself doesn't seem to guarantee that.

sopel39 · 2018-11-16T21:54:09Z

@mbasmanova I think in Presto we rely that immutable collections preserve insertion order quite a lot as it allows for plan determinism. That's why we use Guava immutable collections: Except for sorted collections, order is preserved from construction time. (https://github.com/google/guava/wiki/ImmutableCollectionsExplained).
I've fixed equality inference with that assumption too: a23c88c

mbasmanova · 2018-11-16T21:58:42Z

@sopel39 Karol, I didn't know that. Thanks for explaining.

mbasmanova · 2019-02-14T18:08:03Z

Superseded by #12332

facebook-github-bot added the CLA Signed label Nov 12, 2018

bmc99 force-pushed the stable_explain_plan branch 3 times, most recently from 65e4ce4 to d92de39 Compare November 13, 2018 01:44

kokosing reviewed Nov 13, 2018

View reviewed changes

bmc99 force-pushed the stable_explain_plan branch from d92de39 to 9ffd10b Compare November 13, 2018 06:56

bmc99 changed the title ~~[WIP] Make query plan stable~~ Improve query plan by ordering partition columns Nov 13, 2018

bmc99 force-pushed the stable_explain_plan branch 3 times, most recently from de56f0e to cf20092 Compare November 13, 2018 07:58

bmc99 force-pushed the stable_explain_plan branch from cf20092 to f1d3d25 Compare November 13, 2018 16:31

mbasmanova self-assigned this Nov 13, 2018

kokosing approved these changes Nov 13, 2018

View reviewed changes

electrum reviewed Nov 14, 2018

View reviewed changes

Comment thread presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java Outdated

electrum reviewed Nov 14, 2018

View reviewed changes

Comment thread presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java Outdated

sopel39 reviewed Nov 15, 2018

View reviewed changes

bmc99 changed the title ~~Improve query plan by ordering partition columns~~ Improve explain by ordering predicates Nov 16, 2018

bmc99 force-pushed the stable_explain_plan branch from f1d3d25 to 575b709 Compare November 16, 2018 10:07

mbasmanova reviewed Nov 16, 2018

View reviewed changes

Comment thread presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java Outdated

Print pushed-down predicates in order in explain plans

e3d6e5a

To improve explain plan stability (e.g. make sure explain plans for the same query match) print pushed-down predicates in alphabetical order.

bmc99 force-pushed the stable_explain_plan branch from 575b709 to e3d6e5a Compare November 16, 2018 18:33

mbasmanova approved these changes Nov 16, 2018

View reviewed changes

martint suggested changes Nov 16, 2018

View reviewed changes

mbasmanova closed this Feb 14, 2019

Conversation

bmc99 commented Nov 12, 2018 • edited by mbasmanova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova commented Nov 13, 2018

Uh oh!

bmc99 commented Nov 13, 2018

Uh oh!

martint commented Nov 13, 2018

Uh oh!

mbasmanova commented Nov 13, 2018

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martint left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova commented Nov 16, 2018

Uh oh!

sopel39 commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbasmanova commented Nov 16, 2018

Uh oh!

mbasmanova commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bmc99 commented Nov 12, 2018 •

edited by mbasmanova

Loading

sopel39 Nov 15, 2018 •

edited

Loading

sopel39 commented Nov 16, 2018 •

edited

Loading

mbasmanova commented Feb 14, 2019 •

edited

Loading